router: fixes a regression where headers were not available in request_headers_to_add #41801
Conversation
…t_headers_to_add Signed-off-by: Rohit Agrawal <[email protected]>
|
As I commented in the #39617, The trusted solution is use the For #41794, sure we can did a fix, but use the upstream filter would be better to avoid the behavior be changed again. |
|
As I commented in the #39617, we have no constraint on the order of headers mutations and other operations in the router filter That's say, the request_headers_to_add will be handled by router filter at the end of the downstream HTTP filter chain, but the order between request_headers_to_add and a series of x-envoy-* injections in the router filter are undetermined. So, we should never depends on that, the order may be changed because new feature, bug fix and so on. The trusted solution is use the http_mutation filter in the downstream filter or the upstream filter. Their orders are guaranteed by the filter chain order. For #41794, sure we can did a fix, but use the upstream filter would be better to avoid the behavior be changed again in the future. I think better solution is move the other |
Signed-off-by: Rohit Agrawal <[email protected]>
Signed-off-by: Rohit Agrawal <[email protected]>
|
@wbpcode Thanks for the review and sharing details. Very helpful :) I'll try to move the other |
wbpcode
left a comment
wbpcode
left a comment
There was a problem hiding this comment.
LGTM overall. Thanks for the update and one comment was left. :)
/wait
source/common/router/router.cc
Outdated
| // Set router-set headers before host selection so request_headers_to_add can reference them. | ||
| if (include_attempt_count_in_request_) { | ||
| downstream_headers_->setEnvoyAttemptCount(attempt_count_); | ||
| } | ||
|
|
||
| if (include_timeout_retry_header_in_request_) { | ||
| downstream_headers_->setEnvoyIsTimeoutRetry(is_timeout_retry == TimeoutRetry::Yes ? "true" | ||
| : "false"); | ||
| } | ||
|
|
||
| // Update timeout headers for the retry attempt. | ||
| std::chrono::milliseconds elapsed_time = std::chrono::milliseconds::zero(); | ||
| if (DateUtil::timePointValid(downstream_request_complete_time_)) { | ||
| Event::Dispatcher& dispatcher = callbacks_->dispatcher(); | ||
| elapsed_time = std::chrono::duration_cast<std::chrono::milliseconds>( | ||
| dispatcher.timeSource().monotonicTime() - downstream_request_complete_time_); | ||
| } | ||
|
|
||
| FilterUtility::setTimeoutHeaders(elapsed_time.count(), timeout_, *route_entry_, | ||
| *downstream_headers_, !config_->suppress_envoy_headers_, | ||
| grpc_request_, hedging_params_.hedge_on_per_try_timeout_); | ||
|
|
||
| // Apply request_headers_to_add now that router-set headers are present. | ||
| const Formatter::Context header_transform_context(downstream_headers_, {}, {}, {}, {}, | ||
| &callbacks_->activeSpan()); | ||
| route_entry_->finalizeRequestHeaders(*downstream_headers_, header_transform_context, | ||
| callbacks_->streamInfo(), !config_->suppress_envoy_headers_); | ||
|
|
There was a problem hiding this comment.
When doing the retry, we may shouldn't call the finalizeRequestHeaders again because it may result in unexpected result. For example, if one of the header mutation is to add a new header, because the same header map object is used for retry, the same header may be added for multiple times which is unexpected result. (Although the users may want to map attempt count or timeout header to other headers, it's a corner case anyway. But adding a new normal header is a common case, we should keep the common case works properly first.)
So, IMO, we needn't make any code change to the doRetry and continueDoRetry
There was a problem hiding this comment.
Make sense, I'll remove it for the retry path. Thanks!
Signed-off-by: Rohit Agrawal <[email protected]>
2 participants
Description
This PR fixes a regression where router-set headers like
x-envoy-expected-rq-timeout-ms,x-envoy-attempt-count, etc. were not accessible inrequest_headers_to_addconfiguration.It happened as a side effect of #39617 which moved the call to
route_entry_->finalizeRequestHeaders()earlier in the request processing workflow, which changed whenrequest_headers_to_addis processed.Fix #41794
Commit Message: router: fixes a regression where headers were not available in request_headers_to_add
Additional Description: Fixes a regression where router-set headers like
x-envoy-expected-rq-timeout-ms,x-envoy-attempt-count, etc. were not accessible inrequest_headers_to_addconfiguration.Risk Level: Low
Testing: Added Unit + Integration Tests
Docs Changes: Added
Release Notes: Added